feat(database): CRUD benchmark domain with Postgres - #274
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor License AgreementAll contributors are covered by a CLA. |
| } catch (error) { | ||
| try { | ||
| await client.delete(id); | ||
| } catch { | ||
| // Best-effort cleanup after a failed cycle. | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🟡 A failed database cycle can hang the whole benchmark forever during cleanup
The leftover record is removed (client.delete(id) at benchmarks/database/benchmark.ts:102) with no time limit after a failed cycle, so an unresponsive database leaves the benchmark stuck with no way to finish.
Impact: A single hung cleanup call blocks the entire run indefinitely instead of failing the iteration and moving on.
Why the timeout protection is missing on this path
Every timed phase is wrapped by withTimeout through the step shim in benchmarks/database/crud.bench.ts:77, but the best-effort cleanup inside runCrudCycle's catch block calls the client directly, bypassing that wrapper. The equivalent storage benchmark explicitly wraps its failure-path cleanup: benchmarks/storage/storage.bench.ts:107-108 uses withTimeout(storage!.delete(key), 10_000, 'Delete timed out').
Because the Postgres pool is configured with max: 1 (benchmarks/database/postgres.ts:24), a cleanup delete that never resolves also blocks the single connection for all remaining iterations.
Prompt for agents
In benchmarks/database/benchmark.ts, the catch block of runCrudCycle performs a best-effort cleanup delete by calling client.delete(id) directly, with no timeout. All timed phases go through the step shim in benchmarks/database/crud.bench.ts which wraps calls in withTimeout, so this cleanup is the only unbounded database call in the workload. The Postgres client uses a pool with max: 1, so a hung cleanup also starves every later iteration. Consider bounding the cleanup call with the shared withTimeout helper (benchmarks/src/util/timeout.ts), mirroring how benchmarks/storage/storage.bench.ts bounds its failure-path delete at 10s; this may require passing a timeout (or a pre-wrapped cleanup function) into runCrudCycle.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed in b7ec88a. The failure-path cleanup now goes through a ctx.cleanup() wrapper that crud.bench.ts implements with withTimeout(..., 10_000, 'Delete timed out'), matching benchmarks/storage/storage.bench.ts. Kept the helper out of benchmark.ts so the cycle stays runner-agnostic, and cleanup errors (including that timeout) are still swallowed so the original workload error is what gets rethrown.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Summary
New
benchmarks/database/domain measuring a create → read → update → read → delete cycle, with Postgres as the only provider. Same shape asbenchmarks/storage/:types.ts/providers.ts/benchmark.ts/scoring.ts/legacy-results.ts+ a declarativecrud.bench.ts(config+task,bench runowns the entrypoint,--providerselects one provider). Root scriptsbench:databaseandbench:database:postgressit next tobench:storage*. Runs against a plain local Postgres container — no cloud credentials.Abstraction choice: (a), a provider interface local to the domain
There is no database equivalent of
@storagesdk/core, sotypes.tsdefines the smallest interface the workload needs, mirroring storage'screateStorage()with acreateClient()factory on the provider config:Why not something else: publishing a package was out of scope, and a query-level abstraction (raw SQL, or an ORM like Drizzle) would not carry over to MongoDB/Firestore, which is the whole point of the registry. Everything provider-specific — the
pg.Pool, the table DDL, the parameterised statements — lives inpostgres.tsbehind this interface, so adding MongoDB later is one entry inproviders.tsplus one client module, and swapping in a real SDK later means reimplementingcreateClientonly.deletereturning a count rather thanvoidlets the cycle assert the row is gone without an extra untimed round trip.Workload
Each phase is its own
ctx.stepand separately timed intodata(createMs,readMs,updateMs,readAfterUpdateMs,deleteMs,totalMs,payloadBytes), following how storage reportsuploadMs/downloadMs, so the platform can chart per-phase latency. Reads are verified, not just timed: the post-create read must match the written document and the post-update read must observe the newversion/payload, otherwise the iteration fails withDATABASE_ERROR. Payload size is a--payload-sizeflag parsed from argv the same way storage parses--file-size(default 1 KiB).Env vars
DATABASE_POSTGRES_URL(required)DATABASE_BENCH_TABLE(optional, defaultbenchmark_crud)tsconfig.jsongainsbenchmarks/database/**/*.ts— theincludelist is per-domain, so without itpnpm typecheckwould silently skip every new file.Local run
pnpm typecheckpasses. Run end to end against a local Postgres (5433) with a local benchmarks-platform stack (per.agents/skills/local-platform-e2e) as the reporting target; the platform's ClickHouse import of this run also succeeded (imported: 1, failed: 0, records: 10, steps: 50).Task 1 carries the cold-connection cost; the generated
results/database/*.jsonare not committed.Link to Devin session: https://app.devin.ai/sessions/2fd0b29d80ee433eb61bc9d7e015ed80
Requested by: @HeyGarrison